Support ECDSA TSS in external signer #3734

islamaminBitGo · 2023-07-18T17:44:33Z

No description provided.

zahin-mohammad

not ready for review yet

zahin-mohammad · 2023-07-21T15:08:54Z

modules/sdk-core/src/bitgo/wallet/wallet.ts

+      _.isFunction(params.customPaillierModulusGeneratingFunction) &&
+      _.isFunction(params.customKShareGeneratingFunction) &&
+      _.isFunction(params.customMuDeltaShareGeneratingFunction) &&
+      _.isFunction(params.customSShareGeneratingFunction)


Non-blocking: Why can't we use
typeof v === 'function'

no particular reason, i came across this first so just used it.

but yeah i believe it could be used as well.

zahin-mohammad · 2023-07-21T15:09:28Z

modules/sdk-core/src/bitgo/wallet/wallet.ts

@@ -2799,7 +2810,7 @@ export class Wallet implements IWallet {
    }

    try {
-      const signedTxRequest = await this.tssUtils!.signUsingExternalSigner(
+      const signedTxRequest = await this.tssUtils!.signEddsaTssUsingExternalSigner(


Can you assert tssUtils above? Please don't propagate non-null asserts

zahin-mohammad · 2023-07-21T15:09:55Z

modules/sdk-core/src/bitgo/wallet/wallet.ts

+   * @param params signing options
+   */
+  private async signTransactionTssExternalSignerECDSA(
+    params: WalletSignTransactionOptions = {},


Option params should be last.

zahin-mohammad · 2023-07-21T15:10:41Z

modules/sdk-core/src/bitgo/wallet/wallet.ts

+    let txRequestId = '';
+    if (params.txRequestId) {
+      txRequestId = params.txRequestId;
+    } else if (params.txPrebuild && params.txPrebuild.txRequestId) {
+      txRequestId = params.txPrebuild.txRequestId;
+    } else {
+      throw new Error('TxRequestId required to sign TSS transactions with External Signer.');
+    }
+
+    if (!params.customPaillierModulusGeneratingFunction) {
+      throw new Error('Generator function for paillier modulus required to sign transactions with External Signer.');
+    }
+
+    if (!params.customKShareGeneratingFunction) {
+      throw new Error('Generator function for K share required to sign transactions with External Signer.');
+    }
+
+    if (!params.customMuDeltaShareGeneratingFunction) {
+      throw new Error('Generator function for MuDelta share required to sign transactions with External Signer.');
+    }
+
+    if (!params.customSShareGeneratingFunction) {
+      throw new Error('Generator function for S share required to sign transactions with External Signer.');
+    }


Nit: it would nice to extract this to its own util function and have unit tests just for this.

zahin-mohammad · 2023-07-21T15:10:54Z

modules/sdk-core/src/bitgo/wallet/wallet.ts

+    }
+
+    try {
+      const signedTxRequest = await this.tssUtils!.signEcdsaTssUsingExternalSigner(


Don't propagate non-null asserts

zahin-mohammad · 2023-07-21T16:05:18Z

modules/express/src/clientRoutes.ts

+      switch (req.params.sharetype) {
+        case 'commitment':
+          return await eddsaUtils.createCommitmentShareFromTxRequest(req.body);
+        case 'R':


Please define these magic strings.

ex. ShareType.R

zahin-mohammad · 2023-07-21T16:05:50Z

modules/express/src/clientRoutes.ts

-      customGShareGeneratingFunction: createCustomGShareGenerator(req.config.externalSignerUrl, req.params.coin),
-    };
+    const coin = req.bitgo.coin(req.params.coin);
+    if (coin.getMPCAlgorithm() === 'eddsa') {


Same comment as above.

zahin-mohammad · 2023-07-21T16:05:58Z

modules/express/src/clientRoutes.ts

+        customRShareGeneratingFunction: createCustomRShareGenerator(req.config.externalSignerUrl, req.params.coin),
+        customGShareGeneratingFunction: createCustomGShareGenerator(req.config.externalSignerUrl, req.params.coin),
+      };
+    } else if (coin.getMPCAlgorithm() === 'ecdsa') {


Same comment as above.

zahin-mohammad · 2023-07-21T16:07:09Z

modules/express/test/unit/clientRoutes/externalSign.ts

@@ -102,6 +109,208 @@ describe('External signer', () => {
    envStub.restore();
  });

+  it('should read an encrypted prv from signerFileSystemPath and pass it to PaillierModulus, K, MuDelta, and S share generators', async () => {


Any non-happy path tests we can add? What about invalid inputs/configs?

there are a lot of missing non-happy path tests for the already supported eddsa external signer as well, i created this ticket to add non-happy path tests for external signer in general cause i think it would make this PR way too big. https://bitgoinc.atlassian.net/browse/HSM-120

zahin-mohammad · 2023-07-21T16:12:52Z

modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts

+    challenges: {
+      enterpriseChallenge: EcdsaTypes.SerializedEcdsaChallenges;
+      bitgoChallenge: TxRequestChallengeResponse;
+    };
+    prv: string;
+    derivationPath: string;
+    walletPassphrase?: string;


Nit: for large types, don't inline them

zahin-mohammad · 2023-07-21T16:14:15Z

modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts

    )) as DShare;

    const userSShare = await ECDSAMethods.createUserSignatureShare(
-      userOmicronAndDeltaShare.oShare,
+      step2Return.oShare as OShare,


Why are we casting?

in the case of the hot wallet flow this step2Return would hold the actual OShare which is an OShare object, in the case of external signer it would return an encryptedOShare as a string. This is explained as a comment on the type definition of step2Return.

We can have better types instead of casting. Casting is almost never the fix for production app code.

zahin-mohammad

Please address comments in a follow up

Ticket: HSM-110

islamaminBitGo force-pushed the HSM-110-external-signer-tss-ecdsa branch 3 times, most recently from d3abd9a to 62304fc Compare July 19, 2023 21:32

islamaminBitGo marked this pull request as ready for review July 19, 2023 21:33

islamaminBitGo requested review from a team as code owners July 19, 2023 21:33

zahin-mohammad reviewed Jul 19, 2023

View reviewed changes

zahin-mohammad marked this pull request as draft July 19, 2023 22:26

islamaminBitGo force-pushed the HSM-110-external-signer-tss-ecdsa branch 8 times, most recently from 41b7fd8 to 2903436 Compare July 20, 2023 20:25

islamaminBitGo marked this pull request as ready for review July 20, 2023 21:00

islamaminBitGo requested a review from zahin-mohammad July 20, 2023 21:16

zahin-mohammad requested changes Jul 21, 2023

View reviewed changes

zahin-mohammad reviewed Jul 21, 2023

View reviewed changes

islamaminBitGo force-pushed the HSM-110-external-signer-tss-ecdsa branch from 2903436 to 6bf034d Compare July 26, 2023 03:10

islamaminBitGo requested a review from zahin-mohammad July 26, 2023 03:16

islamaminBitGo force-pushed the HSM-110-external-signer-tss-ecdsa branch from 6bf034d to a3205ae Compare July 26, 2023 18:21

zahin-mohammad previously approved these changes Jul 26, 2023

View reviewed changes

islamaminBitGo dismissed zahin-mohammad’s stale review via c70efb5 July 26, 2023 19:27

islamaminBitGo force-pushed the HSM-110-external-signer-tss-ecdsa branch 2 times, most recently from c70efb5 to 3674012 Compare July 26, 2023 20:26

feat(express): support ECDSA TSS in external signer

03356c1

Ticket: HSM-110

islamaminBitGo force-pushed the HSM-110-external-signer-tss-ecdsa branch from 3674012 to 03356c1 Compare July 26, 2023 21:04

islamaminBitGo requested a review from zahin-mohammad July 26, 2023 21:42

zahin-mohammad approved these changes Jul 26, 2023

View reviewed changes

islamaminBitGo merged commit 24c5df6 into master Jul 26, 2023
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ECDSA TSS in external signer #3734

Support ECDSA TSS in external signer #3734

islamaminBitGo commented Jul 18, 2023

zahin-mohammad left a comment

zahin-mohammad Jul 21, 2023

islamaminBitGo Jul 26, 2023

islamaminBitGo Jul 26, 2023

zahin-mohammad Jul 21, 2023

zahin-mohammad Jul 21, 2023

zahin-mohammad Jul 21, 2023

zahin-mohammad Jul 21, 2023

zahin-mohammad Jul 21, 2023

zahin-mohammad Jul 21, 2023

zahin-mohammad Jul 21, 2023

zahin-mohammad Jul 21, 2023

zahin-mohammad Jul 21, 2023

islamaminBitGo Jul 26, 2023 •

edited

Loading

zahin-mohammad Jul 21, 2023

zahin-mohammad Jul 21, 2023

islamaminBitGo Jul 26, 2023

zahin-mohammad Jul 26, 2023

zahin-mohammad left a comment

Support ECDSA TSS in external signer #3734

Support ECDSA TSS in external signer #3734

Conversation

islamaminBitGo commented Jul 18, 2023

zahin-mohammad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islamaminBitGo Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zahin-mohammad left a comment

Choose a reason for hiding this comment

islamaminBitGo Jul 26, 2023 •

edited

Loading